Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Feature] Multidirectional scrolling #1550

Merged
merged 7 commits into from
Jan 9, 2023
Merged

[Feature] Multidirectional scrolling #1550

merged 7 commits into from
Jan 9, 2023

Conversation

bungoboingo
Copy link
Contributor

@bungoboingo bungoboingo commented Nov 19, 2022 •
edited
Loading

This PR adds support for scrolling horizontally in Iced's `Scrollable' widget.

This PR does not create any breaking changes with the current Scrollable API, except for scrollable::snap_to() operation. All other api additions for horizontal scrolling are opt-in!

multi_directional_scrolling_demo_tiny.mov

Tasks:

  • Update Scrollable to include horizontal scrolling capabilities
  • Update scrollable demo to reflect new changes
  • Introduce styling support for horizontal scrollbar independently of vertical scrollbar

Known issues:

  • There is currently an issue where if you use the snap_to() operation on a Scrollable and don't trigger any action which requests a redraw, the operation will not be reflected until the next redraw. I'm pretty sure this is just inherent to operations and not something that occurred due to my changes, but I'm not 100% certain.

Implementation Details:

  • I've moved around several items in the internal state of a Scrollable widget in order to prevent instances where the offset of the Scrollable could be adjusted independently of the actual status of the scrollbar, leading to potential out of sync states. I moved Scrollbar into scrollable::State as Option<Scrollbar> for both the x & y scrollbars, and moved some methods that once existed in scrollable::State into Scrollbar & Scroller. This does increase the memsize of the internal Scrollable state very slightly. Let me know what you think of these changes! Definitely open to feedback!!
  • Currently styling support for horizontal scrollbars will default to using whatever style is in place for vertical scrollbars, with optional customization.
  • I've updated the snap_to() operation to take a Vector<f32> vs f32 to reflect ability to scroll on both axis.

Behavior Details:

  • Horizontal scrollbars will always be rendered so that if a vertical scrollbar is present, it will adjust its width to always be shunted to the left so that no overlap will occur. You can see this in action in the video I posted above. This made the most sense to me and seems to be how literally every UI widget framework does it.
  • Some UI frameworks (e.g. browsers rendering overflow-y & overflow-x) render a weird little "dead space" box in between horizontal & vertical scrollbars when they are present, which I omitted here. I just am rendering nothing there. Let me know if you think something should be put there, or if this option should be configurable!

Very much open to feedback! Let me know what you think, thanks!

@bungoboingo bungoboingo force-pushed the feat/multidirectional-scrolling branch 3 times, most recently from 2f7c7de to 9d786ee Compare November 19, 2022 19:35
@hecrj hecrj added feature New feature or request widget labels Nov 29, 2022
@hecrj hecrj added this to the 0.6.0 milestone Nov 29, 2022
@jaunkst
Copy link

jaunkst commented Nov 30, 2022

Looking forward to this feature landing. I've been working off this branch for my project and touch events are working as expected.

hecrj
hecrj requested changes Nov 30, 2022
View reviewed changes
Copy link
Member

@hecrj hecrj left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking great! Nice work 🥳 This was long overdue, thanks!

I left some feedback here and there that I think should help simplfiy some of the code. Let me know what you think!

examples/scrollable/Cargo.toml Outdated Show resolved Hide resolved
examples/scrollable/src/main.rs Outdated Show resolved Hide resolved
examples/scrollable/src/main.rs Outdated Show resolved Hide resolved
examples/scrollable/src/main.rs Show resolved Hide resolved
examples/scrollable/src/main.rs Show resolved Hide resolved
native/src/widget/scrollable.rs Outdated Show resolved Hide resolved
native/src/widget/scrollable.rs Outdated Show resolved Hide resolved
native/src/widget/scrollable.rs Outdated Show resolved Hide resolved
native/src/widget/scrollable.rs Outdated Show resolved Hide resolved
native/src/widget/scrollable.rs Outdated Show resolved Hide resolved
@hecrj hecrj modified the milestones: 0.6.0, 0.7.0 Dec 7, 2022
@lfrancke lfrancke mentioned this pull request Dec 15, 2022
Closed
@lfrancke
Copy link

Thanks a lot for this. I did not look at the code but I did test it in my application and it seems to work fine. I also tried touch control (scrolling horizontally by swiping) and that worked (at least on Windows).

@bungoboingo bungoboingo force-pushed the feat/multidirectional-scrolling branch 2 times, most recently from 8e52a6e to e0d6fa9 Compare December 30, 2022 02:24
@bungoboingo
Reworked Scrollable to account for lack of widget order guarantees.
9f85e0c 
Fixed thumb "snapping" bug on scrollable when cursor is out of bounds.
@bungoboingo bungoboingo force-pushed the feat/multidirectional-scrolling branch from e0d6fa9 to 9f85e0c Compare December 30, 2022 02:29
@bungoboingo bungoboingo requested a review from hecrj December 30, 2022 02:33
@bungoboingo
Copy link
Contributor Author

I can't test this on Windows atm, seeking assistance 🙏

@lfrancke
Copy link

Happy to try, anything specific you have in mind?

Just running tests or something more elaborate?

@bungoboingo
Copy link
Contributor Author

Just running the scrollable example on Windows to make sure behavior is as expected. If anyone could test more touch-based multi scrolling platforms would also be appreciated. I don't suspect any issues but would like to confirm before its merged 🙏

@bungoboingo bungoboingo changed the title Feat: Multidirectional scrolling [Feature] Multidirectional scrolling Jan 4, 2023
@lfrancke
Copy link

lfrancke commented Jan 5, 2023 •
edited
Loading

I tried it today on a Windows laptop (Surface) using touch-screen and it worked beautifully.

There is one thing that's a bit weird and I'll try to describe, maybe that's just a feeling though:

  • When I touch the screen/content (not scrollbar) and move my finger to the right, everything moves left (including the scrollbar) -> This is just as I'd expect
  • When I touch the scrollbar itself, the same thing happens. What I expect is different though. I want to "drag" the scrollbar thing to the left but it actually moves to the right.

This is different than it works in other UIs. There it works as expected: Drag the content and it moves the opposite direction, drag the scrollbar itself and it moves along with your finger.

I hope that makes sense?
I also tried to cross-compile to a Raspberry but the compilation failed (unrelated to this), I'll try again later.
This looks very good though!

Update: The behavior is the same on horizontal and vertical scrolling so probably nothing to fix in this PR

hecrj
hecrj approved these changes Jan 8, 2023
View reviewed changes
Copy link
Member

@hecrj hecrj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome stuff @bungoboingo! Looks great 🥳

I made some smaller changes here and there to bring it to the finish line. Let me know what you think, and I believe we can merge!

@lfrancke I believe I have fixed this behavior you mention in 2d00747.

@bungoboingo
Copy link
Contributor Author

LGTM! Tested the changes and everything appears to be working as intended. TIL shift + mousewheel scrolls horizontally on browsers 🤯

@hecrj hecrj merged commit 7ccd87c into iced-rs:master Jan 9, 2023
@hecrj
Copy link
Member

hecrj commented Jan 9, 2023

@bungoboingo Life changer! You are welcome :D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hecrj hecrj hecrj approved these changes

Assignees
No one assigned
Labels
feature New feature or request widget
Projects
None yet
Milestone
0.7.0
Development

Successfully merging this pull request may close these issues.
4 participants
@bungoboingo @jaunkst @lfrancke @hecrj